Read messages before applying quota to avoid mplex backpressure issues #4697
Conversation
Menduist
changed the title
| # limited by libp2p), this will naturally adapt them to the available | ||
| # quota. | ||
|
|
||
| awaitQuota(peer, libp2pRequestCost, shortProtocolId(protocolId)) |
There was a problem hiding this comment.
hm, it's ok to read before quota, but we must count failures also - ie receiving an invalid message should also count / be slowed down - this protects against buggy clients tight-looping an invalid request
There was a problem hiding this comment.
ping @Menduist what shall we do with this PR?
There was a problem hiding this comment.
Sorry forgot about it, I'll find a way to take your comment into account and we should merge it
|
A possible issue with this PR is that we may hold 10 "request object" in memory while we wait for quota, whereas before, in this scenario, the data would have been queued in the kernel instead (so not decompressed etc) |
this is fine since the number of concurrent requests is reasonably capped - that said, there's an old issue open about computing the maximum size of an SSZ message based on its limits which would further cap the damage - it's already possible for fixed-size messages (which most requests are) so it would be good to check that as well. cc @zah |
@Menduist in the meantime, this deserves an explanatory comment in the code |
| # limited by libp2p), this will naturally adapt them to the available | ||
| # quota. | ||
|
|
||
| awaitQuota(peer, libp2pRequestCost, shortProtocolId(protocolId)) |
There was a problem hiding this comment.
also, we need an explanation here why it's done in finally, which is .. unusual
With mplex, the backpressure is at the connection level, and not the stream level.
It means that if a received message is not read, the entire connection will get blocked
This PR modifies the quota to be applied after reading the message, and not before, since doing so will pause the entire connection
The number of simultaneous requests is still limited by libp2p
cc @arnetheduck